Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Name some numeric constants #27

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vincentbernat
Copy link
Contributor

240 is the minimum size of a DHCP packet. Since it is used at various
places, use a named constant instead. It is also exposed to the
user. The DHCP magic cookie also gets a named variable.

240 is the minimum size of a DHCP packet. Since it is used at various
places, use a named constant instead. It is also exposed to the
user. The DHCP magic cookie also gets a named variable.
@krolaw
Copy link
Owner

krolaw commented Oct 22, 2016

240 is really the Options start point, and calling it Minimal packet size where it relates to Option handling (which is all but one time) would in my opinion cause confusion. It could be a private const inside server.go where only that 240 is changed (that may increase clarity, but the comment explains the purpose anyway). If 240 is turned into a const inside packet.go, it should be called something else (i.e OptionStart), and then all the other start points would have to follow - which I don't believe is necessary or helpful.

Magic cookie, IMHO need not to be exposed to the user. If it needs to checked, for received message validation, then that check should be added to server.go, in which case a var may be beneficial.

@vincentbernat
Copy link
Contributor Author

In my case, I don't use server.go, just packet.go. And if the packet is too short, everything panics. I am OK with changing the name, OptionStart or BaseSize or BootpSize.

As for the cookie, same as above, I don't user server.go. The check could be added (and should since otherwise, we may not have a DHCP packet, but maybe a BOOTP one or something else) but if the variable could be made public, this would help me.

@krolaw
Copy link
Owner

krolaw commented Oct 22, 2016

I've very cautious about making changes for convenience (now that large organisations are using it), especially when adding two lines to your own code will solve your issue. It's unfortunate, my library wasn't designed like dhcpr, but what's done is done.

I'm leaving this open to think on this some more.

@vincentbernat
Copy link
Contributor Author

The change is about adding constants. This won't break any existing code (unlike other changes, like not sending the lease time anymore).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants